Decouple `BackgroundColor` from `UiImage` by benfrankel · Pull Request #11165 · bevyengine/bevy

benfrankel · 2024-01-01T01:54:47Z

Objective

Solution

Stop using BackgroundColor as a color tint for UiImage. Add a UiImage::color field for color tint instead. Allow a UI node to simultaneously include a solid-color background and an image, with the image rendered on top of the background (this is already how it works for e.g. text).

Changelog

The BackgroundColor component now renders a solid-color background behind UiImage instead of tinting its color.
Removed BackgroundColor from ImageBundle, AtlasImageBundle, and ButtonBundle.
Added UiImage::color.
Expanded RenderUiSystem variants.
Renamed bevy_ui::extract_text_uinodes to extract_uinodes_text for consistency.

Migration Guide

BackgroundColor no longer tints the color of UI images. Use UiImage::color for that instead.
For solid color buttons, replace ButtonBundle { background_color: my_color.into(), ... } with ButtonBundle { image: UiImage::default().with_color(my_color), ... }, and update button interaction systems to use UiImage::color instead of BackgroundColor as well.
bevy_ui::RenderUiSystem::ExtractNode has been split into ExtractBackgrounds, ExtractImages, ExtractBorders, and ExtractText.
bevy_ui::extract_uinodes has been split into extract_uinode_background_colors and extract_uinode_images.
bevy_ui::extract_text_uinodes has been renamed to extract_uinode_text.

benfrankel · 2024-01-01T01:59:47Z

I'm not sure the changelog / migration guides are written the way they should be / covering what they should.

Also, I removed Without<ContentSize> from the UI node query in extract_uinode_border so that I could add a border to the ui_texture_atlas example, but there may be a good reason for it that I'm not aware of (@ickshonpe ?).

benfrankel · 2024-01-01T02:04:08Z

Also I'm curious if there's a reason that ExtractedUiNodes::uinodes is a hash map with Entity key instead of just a Vec. The code isn't straightforward so I couldn't tell how the Entity gets used. Most of the extract_uinode_xyz systems just create a new entity with commands.spawn_empty().id() anyways, and now only extract_uinode_background uses the actual original UI node entity as a key.

pablo-lua · 2024-01-01T10:54:10Z

isn't the Entity used in bevy_ui\src\render\mod.rs by the Render phase?
Btw, good changes, the examples probably will change too.

ickshonpe · 2024-01-01T11:37:18Z

I put in a similar PR a year ago #7451 but it got no reviews.
I'm happy to close that though in favour of this one, as this one targets the latest bevy.

I think as well in my PR the colour was a field on the UiImage type, which isn't as flexible as a separate component and makes change detection awkward.

pablo-lua · 2024-01-01T11:41:39Z

I think as well in my PR the colour was a field on the UiImage type, which isn't as flexible as a separate component and makes change detection awkward.

We can probably solve that if we go with the ColorTint idea in the future

ickshonpe · 2024-01-01T11:56:39Z

I think as well in my PR the colour was a field on the UiImage type, which isn't as flexible as a separate component and makes change detection awkward.

We can probably solve that if we go with the ColorTint idea in the future

Ah yeah I wrote that before I started my review, I assumed we were going with a separate component for the color.

crates/bevy_ui/src/render/mod.rs

crates/bevy_ui/src/ui_node.rs

crates/bevy_ui/src/render/mod.rs

ickshonpe · 2024-01-01T12:51:03Z

I'm not sure the changelog / migration guides are written the way they should be / covering what they should.

Also, I removed Without<ContentSize> from the UI node query in extract_uinode_border so that I could add a border to the ui_texture_atlas example, but there may be a good reason for it that I'm not aware of (@ickshonpe ?).

ContentSize uses different layout rules and may not work correctly with borders in all cases, I can't remember though it's been a while since I tested it. I think the filter should remain for now.

You should be able to display atlas images with a border with a construction like this I think

 commands.spawn((
        NodeBundle {
            style: Style {
                border: UiRect::all(Val::Px(20.)),
                ..Default::default()
            },
            border_color: Color::RED.into(),
            ..Default::default()
        }, 
        UiTextureAtlasImage { index, ..Default::default() }, 
        texture_atlas_handle
    ));

Or you could use an Outline instead of a border.

ickshonpe · 2024-01-01T13:15:41Z

Also I'm curious if there's a reason that ExtractedUiNodes::uinodes is a hash map with Entity key instead of just a Vec. The code isn't straightforward so I couldn't tell how the Entity gets used. Most of the extract_uinode_xyz systems just create a new entity with commands.spawn_empty().id() anyways, and now only extract_uinode_background uses the actual original UI node entity as a key.

I made a couple of PRs improving UI extraction performance, but there were a lot of changes to rendering for Bevy 0.12, and not all of them have been merged yet so things are in kind of an inbetween state. Some of them were: #9212 #9668 #9889 #9853 #9877

alice-i-cecile · 2024-01-24T16:47:35Z

@benfrankel I'd like to review this and get it merged: can you resolve merge conflicts?

benfrankel · 2024-01-24T23:11:18Z

@alice-i-cecile done. Note that I effectively reverted #11205 because with background color and images being extracted separately, skipping loading images won't also skip their background color. I might have misunderstood the issue there though, in which case I can remove the skip again.

alice-i-cecile · 2024-01-24T23:22:20Z

@alice-i-cecile done. Note that I effectively reverted #11205 because with background color and images being extracted separately, skipping loading images won't also skip their background color. I might have misunderstood the issue there though, in which case I can remove the skip again.

Thanks! That's a question for @JMS55 and/or @mockersf.

crates/bevy_ui/src/render/mod.rs

ManevilleF · 2024-02-29T22:01:18Z

You are absolutely right, we need to schedule a cleanup to fix inconsistent components in bundles, for this PR I think we can focus on iso-functionality

pablo-lua

I think this is a good update and the cleanup can be done later on. All good

# Objective - After #11165, example `ui` is not pretty as it displays the Bevy logo on a white background, with a comment that is now wrong ## Solution - Remove the background color

# Objective Fixes bevyengine#11157. ## Solution Stop using `BackgroundColor` as a color tint for `UiImage`. Add a `UiImage::color` field for color tint instead. Allow a UI node to simultaneously include a solid-color background and an image, with the image rendered on top of the background (this is already how it works for e.g. text). ![2024-02-29_1709239666_563x520](https://github.com/bevyengine/bevy/assets/12173779/ec50c9ef-4c7f-4ab8-a457-d086ce5b3425) --- ## Changelog - The `BackgroundColor` component now renders a solid-color background behind `UiImage` instead of tinting its color. - Removed `BackgroundColor` from `ImageBundle`, `AtlasImageBundle`, and `ButtonBundle`. - Added `UiImage::color`. - Expanded `RenderUiSystem` variants. - Renamed `bevy_ui::extract_text_uinodes` to `extract_uinodes_text` for consistency. ## Migration Guide - `BackgroundColor` no longer tints the color of UI images. Use `UiImage::color` for that instead. - For solid color buttons, replace `ButtonBundle { background_color: my_color.into(), ... }` with `ButtonBundle { image: UiImage::default().with_color(my_color), ... }`, and update button interaction systems to use `UiImage::color` instead of `BackgroundColor` as well. - `bevy_ui::RenderUiSystem::ExtractNode` has been split into `ExtractBackgrounds`, `ExtractImages`, `ExtractBorders`, and `ExtractText`. - `bevy_ui::extract_uinodes` has been split into `bevy_ui::extract_uinode_background_colors` and `bevy_ui::extract_uinode_images`. - `bevy_ui::extract_text_uinodes` has been renamed to `extract_uinode_text`.

# Objective - After bevyengine#11165, example `ui` is not pretty as it displays the Bevy logo on a white background, with a comment that is now wrong ## Solution - Remove the background color

# Objective - Since #11165, the button in the mobile example doesn't visually react to touches ## Solution - Correctly set up the background color

# Objective - After bevyengine#11165, example `ui` is not pretty as it displays the Bevy logo on a white background, with a comment that is now wrong ## Solution - Remove the background color

@viridia

…tuitive (#14017) # Objective In Bevy 0.13, `BackgroundColor` simply tinted the image of any `UiImage`. This was confusing: in every other case (e.g. Text), this added a solid square behind the element. #11165 changed this, but removed `BackgroundColor` from `ImageBundle` to avoid confusion, since the semantic meaning had changed. However, this resulted in a serious UX downgrade / inconsistency, as this behavior was no longer part of the bundle (unlike for `TextBundle` or `NodeBundle`), leaving users with a relatively frustrating upgrade path. Additionally, adding both `BackgroundColor` and `UiImage` resulted in a bizarre effect, where the background color was seemingly ignored as it was covered by a solid white placeholder image. Fixes #13969. ## Solution Per @viridia's design: > - if you don't specify a background color, it's transparent. > - if you don't specify an image color, it's white (because it's a multiplier). > - if you don't specify an image, no image is drawn. > - if you specify both a background color and an image color, they are independent. > - the background color is drawn behind the image (in whatever pixels are transparent) As laid out by @benfrankel, this involves: 1. Changing the default `UiImage` to use a transparent texture but a pure white tint. 2. Adding `UiImage::solid_color` to quickly set placeholder images. 3. Changing the default `BorderColor` and `BackgroundColor` to transparent. 4. Removing the default overrides for these values in the other assorted UI bundles. 5. Adding `BackgroundColor` back to `ImageBundle` and `ButtonBundle`. 6. Adding a 1x1 `Image::transparent`, which can be accessed from `Assets<Image>` via the `TRANSPARENT_IMAGE_HANDLE` constant. Huge thanks to everyone who helped out with the design in the linked issue and [the Discord thread](https://discord.com/channels/691052431525675048/1255209923890118697/1255209999278280844): this was very much a joint design. @cart helped me figure out how to set the UiImage's default texture to a transparent 1x1 image, which is a much nicer fix. ## Testing I've checked the examples modified by this PR, and the `ui` example as well just to be sure. ## Migration Guide - `BackgroundColor` no longer tints the color of images in `ImageBundle` or `ButtonBundle`. Set `UiImage::color` to tint images instead. - The default texture for `UiImage` is now a transparent white square. Use `UiImage::solid_color` to quickly draw debug images. - The default value for `BackgroundColor` and `BorderColor` is now transparent. Set the color to white manually to return to previous behavior.

@viridia

…tuitive (#14017) # Objective In Bevy 0.13, `BackgroundColor` simply tinted the image of any `UiImage`. This was confusing: in every other case (e.g. Text), this added a solid square behind the element. #11165 changed this, but removed `BackgroundColor` from `ImageBundle` to avoid confusion, since the semantic meaning had changed. However, this resulted in a serious UX downgrade / inconsistency, as this behavior was no longer part of the bundle (unlike for `TextBundle` or `NodeBundle`), leaving users with a relatively frustrating upgrade path. Additionally, adding both `BackgroundColor` and `UiImage` resulted in a bizarre effect, where the background color was seemingly ignored as it was covered by a solid white placeholder image. Fixes #13969. ## Solution Per @viridia's design: > - if you don't specify a background color, it's transparent. > - if you don't specify an image color, it's white (because it's a multiplier). > - if you don't specify an image, no image is drawn. > - if you specify both a background color and an image color, they are independent. > - the background color is drawn behind the image (in whatever pixels are transparent) As laid out by @benfrankel, this involves: 1. Changing the default `UiImage` to use a transparent texture but a pure white tint. 2. Adding `UiImage::solid_color` to quickly set placeholder images. 3. Changing the default `BorderColor` and `BackgroundColor` to transparent. 4. Removing the default overrides for these values in the other assorted UI bundles. 5. Adding `BackgroundColor` back to `ImageBundle` and `ButtonBundle`. 6. Adding a 1x1 `Image::transparent`, which can be accessed from `Assets<Image>` via the `TRANSPARENT_IMAGE_HANDLE` constant. Huge thanks to everyone who helped out with the design in the linked issue and [the Discord thread](https://discord.com/channels/691052431525675048/1255209923890118697/1255209999278280844): this was very much a joint design. @cart helped me figure out how to set the UiImage's default texture to a transparent 1x1 image, which is a much nicer fix. ## Testing I've checked the examples modified by this PR, and the `ui` example as well just to be sure. ## Migration Guide - `BackgroundColor` no longer tints the color of images in `ImageBundle` or `ButtonBundle`. Set `UiImage::color` to tint images instead. - The default texture for `UiImage` is now a transparent white square. Use `UiImage::solid_color` to quickly draw debug images. - The default value for `BackgroundColor` and `BorderColor` is now transparent. Set the color to white manually to return to previous behavior.

benfrankel force-pushed the background-color branch from 11edee5 to 770d686 Compare January 1, 2024 01:57

alice-i-cecile added this to the 0.13 milestone Jan 1, 2024

alice-i-cecile added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 1, 2024

ickshonpe suggested changes Jan 1, 2024

View reviewed changes

benfrankel force-pushed the background-color branch from 770d686 to c91f0f8 Compare January 1, 2024 19:29

benfrankel force-pushed the background-color branch 2 times, most recently from 172a5c7 to 918867c Compare January 24, 2024 23:00

benfrankel changed the title ~~Decouple BackgroundColor from UiImage and UiTextureAtlasImage~~ Decouple BackgroundColor from UiImage Jan 24, 2024

benfrankel force-pushed the background-color branch from 918867c to 6eeea80 Compare January 24, 2024 23:08

alice-i-cecile requested a review from ickshonpe January 24, 2024 23:22

JMS55 reviewed Jan 24, 2024

View reviewed changes

crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved

benfrankel force-pushed the background-color branch from 6eeea80 to d9f77f3 Compare January 24, 2024 23:34

alice-i-cecile modified the milestones: 0.13, 0.14 Feb 2, 2024

benfrankel force-pushed the background-color branch 3 times, most recently from a2f123a to 3aaf42a Compare February 26, 2024 03:48

pablo-lua approved these changes Feb 29, 2024

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 29, 2024

ManevilleF approved these changes Mar 1, 2024

View reviewed changes

benfrankel added 5 commits March 3, 2024 13:01

Decouple BackgroundColor from UiImage

ef109a9

Use image.color in ComputedTextureSlices::extract_ui_nodes

52b40d8

Re-add UiImage to ButtonBundle, but default to full transparency

87183a0

Remove BackgroundColor from image and button bundles

ea716d9

Fix bevy_color migration issues

94e6deb

benfrankel force-pushed the background-color branch from 636b9f1 to 94e6deb Compare March 3, 2024 21:06

alice-i-cecile added M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Release-Note Work that should be called out in the blog due to impact labels Mar 3, 2024

alice-i-cecile added this pull request to the merge queue Mar 3, 2024

Merged via the queue into bevyengine:main with commit e8ae0d6 Mar 3, 2024

mockersf mentioned this pull request Mar 4, 2024

remove background color from UI example image #12306

Merged

benfrankel deleted the background-color branch March 5, 2024 01:01

mockersf mentioned this pull request Mar 8, 2024

correctly set up background color in mobile example #12384

Merged

ChristopherBiscardi mentioned this pull request Apr 1, 2024

Separate image and background color values #7451

Closed

This was referenced May 30, 2024

Write release notes for PR #11165: Decouple BackgroundColor from UiImage bevyengine/bevy-website#1234

Closed

Write release notes for PR #11165: Decouple BackgroundColor from UiImage bevyengine/bevy-website#1254

Closed

This was referenced Jun 25, 2024

Styling regression: ButtonBundle background color #13969

Closed

Make default behavior for BackgroundColor and BorderColor more intuitive #14017

Merged

Uh oh!

Conversation

benfrankel commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Migration Guide

Uh oh!

benfrankel commented Jan 1, 2024

Uh oh!

benfrankel commented Jan 1, 2024

Uh oh!

pablo-lua commented Jan 1, 2024

Uh oh!

ickshonpe commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablo-lua commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ickshonpe commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ickshonpe commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ickshonpe commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jan 24, 2024

Uh oh!

benfrankel commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jan 24, 2024

Uh oh!

Uh oh!

ManevilleF commented Feb 29, 2024

Uh oh!

pablo-lua left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

benfrankel commented Jan 1, 2024 •

edited

Loading

ickshonpe commented Jan 1, 2024 •

edited

Loading

pablo-lua commented Jan 1, 2024 •

edited

Loading

ickshonpe commented Jan 1, 2024 •

edited

Loading

ickshonpe commented Jan 1, 2024 •

edited

Loading

ickshonpe commented Jan 1, 2024 •

edited

Loading

benfrankel commented Jan 24, 2024 •

edited

Loading